Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add key derivation options support #49

Merged
merged 12 commits into from
Nov 9, 2023

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Oct 24, 2023

This PR adds support for multiple key derivation algorithms and parameters, by modifying the shape of the vault (encrypted data) and the encryption key (also when exported), in order to hold some (nullable) metadata related to how the key was derived in the first place.

This is mainly needed to increase key derivation iterations, while keeping old vaults decryption possible. With this PR, PBKDF2 default iterations has been set to 900.000 - but when the old shape of the vault or key is detected (missing derivation metadata), the old number of iterations is used.

To keep compatibility with older formats, methods exported by this library will accept both old and new formats.

Changes

  • ADDED: EncryptionKey type to hold a CryptoKey along with its derivation parameters

  • ADDED: ExportedEncryptionKey type to hold a JsonWebKey along with its derivation parameters

  • ADDED: Optional keyMetadata property of type KeyDerivationOptions to EncryptionResult

  • ADDED: Optional opts argument to keyFromPassword to specify algorithm and parameters to be used in the key derivation. Defaults to PBKDF2 at 10.000 iterations

  • ADDED: Added iterations argument to keyFromPassword function

  • ADDED: Optional keyDerivationOptions argument to encrypt to specify algorithm and parameters to be used in the key derivation. Defaults to PBKDF2 at 900.000 iterations

  • ADDED: Optional keyDerivationOptions argument to encryptWithDetail to specify algorithm and parameters to be used in the key derivation. Defaults to PBKDF2 at 900.000 iterations

  • ADDED: updateVaultWithDetail function to update existing vault and exported key with a safer encryption method if available

  • ADDED: updateVault function to update existing vault string with a safer encryption method if available

  • CHANGED: encrypt method accepts both EncryptionKey and CryptoKey types as key argument

  • CHANGED: encryptWithKey method accepts both EncryptionKey and CryptoKey types as key argument

  • CHANGED: decrypt method accepts both EncryptionKey and CryptoKey types as key argument

  • CHANGED: decryptWithKey method accepts both EncryptionKey and CryptoKey types as key argument

  • CHANGED: importKey method returns a CryptoKey when a JWK string is passed, or an EncryptionKey when an ExportedEncryptionKey string is passed.

  • CHANGED: exportKey method accepts both EncryptionKey and CryptoKey types as key argument, and returns an ExportedEncryptionKey for the former and a JsonWebKey for the latter.

@mikesposito mikesposito force-pushed the refactor/increase-pbkdf2-iterations branch from 17aedf0 to aeff2b4 Compare October 24, 2023 11:13
@mikesposito mikesposito marked this pull request as ready for review October 24, 2023 11:26
@mikesposito mikesposito requested review from a team as code owners October 24, 2023 11:26
@Mrtenz Mrtenz changed the title Increase PBKDF2 iterations BREAKING: Increase PBKDF2 iterations Oct 24, 2023
NicholasEllul
NicholasEllul previously approved these changes Oct 24, 2023
src/index.ts Outdated
* @returns A CryptoKey for encryption and decryption.
*/
export async function keyFromPassword(
password: string,
salt: string,
exportable = false,
iterations = 900_000,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea, this will allow clients who can handle more iterations to increase this number without harming those which cannot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also gives the chance to a client to decrypt a vault with a weaker key to then re-encrypt it with a more secure one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  iterations = 900_000,

has been changed to:

  opts: KeyDerivationOptions = {
    algorithm: 'PBKDF2',
    params: {
      iterations: 900_000,
      exportable: false,
    },
  },

cryptodev-2s
cryptodev-2s previously approved these changes Oct 24, 2023
Copy link

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kanthesha
Copy link

LGTM!

kanthesha
kanthesha previously approved these changes Oct 24, 2023
src/index.ts Outdated Show resolved Hide resolved
salt: 'sysHvNRoWykN/JVUSpBwXhmp0llTMQabfY7zucEfAJg=',
};

test('encryptor:decrypt encrypted data with key derived with different number of iterations', async ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for extra clarity

Suggested change
test('encryptor:decrypt encrypted data with key derived with different number of iterations', async ({
test('encryptor:decrypt encrypted data using key derived with smaller number of iterations than the default configuration', async ({

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing this test case, and instead repeating all test cases for both types of encryption results (old and new), to better test backward compatibility

MajorLift
MajorLift previously approved these changes Oct 24, 2023
Copy link
Member Author

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to dismiss all approvals as we need a way to know how the key was derived in some scenarios.

For this reason, in most of the functions, instead of accepting / returning a CryptoKey we'll return an EncryptionKey object, which has this shape:

export type EncryptionKey = {
  key: CryptoKey; // this might change in the future for other, more generic, types
  derivationFunction: KeyDerivationOptions;
};

export type KeyDerivationOptions = {
  algorithm: 'PBKDF2'; // this can easily become an enum in the future
  params: PBKDF2Params; // ..and this a union type, depending on `algorithm`
};

export type PBKDF2Params = {
  iterations?: number;
  exportable?: boolean;
};

And keyMetadata field (which reports the same object of derivationFunction) has been added also to the encryption result object:

export type EncryptionResult = {
  data: string;
  iv: string;
  salt?: string;
  // old encryption results will not have this
  keyMetadata?: KeyDerivationOptions;
};

Finally, instead of adding a single iterations argument to keyFromPassword function, the function has now this signature:

keyFromPassword(
  password: string,
  salt: string,
  opts: KeyDerivationOptions = {
    algorithm: 'PBKDF2',
    params: {
      iterations: 900_000,
      exportable: false,
    },
  },
): Promise<EncryptionKey>;

This allows:

  1. The client to choose the preferred algorithm and parameters
  2. To easily extend this in the future with other algorithms and parameters

test/index.spec.ts Show resolved Hide resolved
Copy link
Member Author

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in f10e866 are needed to carry derivation function metadata in the exported key and to expect them (in new cases) when importing it.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
@mikesposito
Copy link
Member Author

This PR will also close #1

Copy link

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do acknowledge the benefit of having easy backwards compatibility, I am worried about the longer term technical debt this may incur. Given we are adding additional guards, conditions, and migrated tests to support two kinds of inputs for each operation, many of the the functions now require additional context on the various support cases to be modified effectively in the future.

IMO this is a case where I think verbosity may be better than the neatness of minimizing bundle size, as software bugs here can result in a bit more severe behaviour. Especially since a majority of the conditions are only relevant on that first migration.

What are your thoughts on this:

  1. We denote this as being a breaking change and define a migration guide for developers who wish to update to this breaking change version.
  2. Alongside the guide, we provide a transformation function which will take in the encrypted data, and transform it into the format we expect. Notably, adding the key argument, and adding the previous parameters that we used to use for the PBKDF2 function. This function will be a no-op if the vault is already migrated.
  3. Consumers of this package will simply call the transformation function after they've loaded their vault, but before they interact with the updated interface.

In the MetaMask extension, this can be done with a migration, in other clients, they can use their own strategy, or simply call this each time their vault is loaded as it will later get saved with the new updated parameters.

This will remove all the additional complexity from these functions leaving them simple to understand and build on in the future without worry of breaking vault compatibility.

src/index.ts Outdated
Comment on lines 6 to 15
export type PBKDF2Params = {
iterations?: number;
exportable?: boolean;
};

export type KeyDerivationOptions = {
algorithm: 'PBKDF2';
params: PBKDF2Params;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This also makes it pretty easy to expand to other algorithms one day doing something like:

export type KeyDerivationOptions = { algorithm: 'PBKDF2'; params: PBKDF2Params;} | { algorithm: 'OtherAlgo'; params: OtherAlgoParams;}

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
Comment on lines 80 to 85
const key = await keyFromPassword(password, salt, {
algorithm: 'PBKDF2',
params: {
exportable: true,
},
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to allow the client to specify its iterations here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! this has been added as an optional argument, with DEFAULT_DERIVATION_PARAMS as the default value in 9d06954

src/index.ts Outdated
Comment on lines 131 to 147
if (
!(encryptionKey instanceof CryptoKey) &&
encryptionKey.derivationFunction
) {
encryptionResult.keyMetadata = encryptionKey.derivationFunction;
}

return encryptionResult;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only conditionally add the metadata to the encryption result?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if the user encrypts data using an arbitrary CryptoKey we can't assume that it was derived with this library, thus we should leave the metadata empty

src/index.ts Outdated
Comment on lines 131 to 147
if (
!(encryptionKey instanceof CryptoKey) &&
encryptionKey.derivationFunction
) {
encryptionResult.keyMetadata = encryptionKey.derivationFunction;
}

return encryptionResult;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only conditionally add the metadata to the encryption result?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if the user encrypts data using an arbitrary CryptoKey we can't assume that it was derived with this library, thus we should leave the metadata empty

src/index.ts Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
@kumavis
Copy link
Member

kumavis commented Oct 27, 2023

didnt read thread yet but want to add that this doesn't have to be breaking if we record the iterations and assume lack of iterations means the present old default

@danfinlay
Copy link
Contributor

Thank you for this, I've wanted to do this since first writing the module.

I suspect there's a way to do this without breaking, and allowing for a smooth migration path for users of the library:

  1. If a vault is restored with no params value, then assume the old default. Maybe log a warning when ever not providing a value.
  2. When encrypting, default to the new iteration default, and include the params value in the payload.

This way, even a consumer who is unaware of this issue could have their vaults silently upgraded to a more secure number of iterations.

@mikesposito
Copy link
Member Author

mikesposito commented Oct 31, 2023

@kumavis @danfinlay Thanks for taking a look into this!

There are two reasons why making this non-breaking or assuming lower iterations might be erroneous:

  1. Assuming that a key was derived with lower iterations might be wrong, as we allow users to use their own CryptoKey, which could have been derived with other libraries / algorithms
  2. Even if we assume that all vaults without metadata are encrypted with PBKDF2 @10.000 iterations, this should be breaking anyway as keyFromPassword will have different default parameters - so users should only update this package voluntarily with a major bump.

Also, when encrypting new vaults, if we don't want to stop accepting arbitrary CryptoKey's, we'll not be able to assume that the key was derived with a certain algorithm or params. This is why, in this PR, the EncryptionResult metadata related to the key derivation is optional.

Please let me know if this makes sense, and if it would make sense to stop accepting arbitrary (with no metadata) keys when encrypting new vaults - given also that what @NicholasEllul wrote is correct: keeping compatibility with older versions increases complexity

@Gudahtt
Copy link
Member

Gudahtt commented Oct 31, 2023

Assuming that a key was derived with lower iterations might be wrong

Perhaps I'm missing something, but I thought we only used these parameters to create the key. So if we have the key already, it doesn't matter how many iterations were used to create it.

Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some other comments besides this but I will save that for another comment so it is more easily quotable. A lot of these comments are just about the tests.

test/index.spec.ts Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@mikesposito mikesposito changed the title BREAKING: Increase PBKDF2 iterations Increase PBKDF2 iterations Nov 6, 2023
@mikesposito mikesposito changed the title Increase PBKDF2 iterations Add key derivation options support Nov 6, 2023
src/index.ts Outdated
Comment on lines 505 to 524
/**
* Updates the provided vault, re-encrypting
* data with a safer algorithm if one is available.
*
* If the provided vault is already using the latest available encryption method,
* it is returned as is.
*
* @param vault - The vault to update.
* @param password - The password to use for encryption.
* @returns A promise resolving to the updated vault.
*/
export async function updateVault(
vault: string,
password: string,
): Promise<string> {
if (isVaultUpdated(vault)) {
return vault;
}

return encrypt(password, await decrypt(password, vault));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can be used by the extension and other clients to migrate their vaults.

As this PR is non-breaking, this is not mandatory, as the library would silently use the new method when encrypting the vault again. But with this helper, the library gets the responsibility of knowing whether the vault is using the latest safe configuration available, resulting in a no-op if the vault is already up-to-date

Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final thing, but this otherwise looks good to me.

yarn.lock Outdated Show resolved Hide resolved
@mikesposito mikesposito force-pushed the refactor/increase-pbkdf2-iterations branch from 65a9009 to c50ede4 Compare November 8, 2023 12:29
@mikesposito mikesposito force-pushed the refactor/increase-pbkdf2-iterations branch from c50ede4 to e1fe87e Compare November 8, 2023 17:04
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Nov 8, 2023
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +448 to +471
/**
* Updates the provided vault and exported key, re-encrypting
* data with a safer algorithm if one is available.
*
* If the provided vault is already using the latest available encryption method,
* it is returned as is.
*
* @param encryptionResult - The encrypted data to update.
* @param password - The password to use for encryption.
* @returns A promise resolving to the updated encrypted data and exported key.
*/
export async function updateVaultWithDetail(
encryptionResult: DetailedEncryptionResult,
password: string,
): Promise<DetailedEncryptionResult> {
if (isVaultUpdated(encryptionResult.vault)) {
return encryptionResult;
}

return encryptWithDetail(
password,
await decrypt(password, encryptionResult.vault),
);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On @metamask/eth-keyring-controller, we use detailed encryption / decryption as we have to cache the encryption key derived from the password. Without this function, when running the extension in MV3-compatible mode, we'll not be able to cache the encryption key if the vault gets updated - the cached encryption key will become invalid/outdated in case the vault gets updated, so we need to return the exported key in these cases.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mikesposito mikesposito merged commit ea07cd2 into main Nov 9, 2023
16 checks passed
@mikesposito mikesposito deleted the refactor/increase-pbkdf2-iterations branch November 9, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants